Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[KYUUBI #5377][FOLLOWUP] Get limit from more spark plan and regard result max rows #5963

Closed
wants to merge 7 commits into from

Conversation

turboFei
Copy link
Member

@turboFei turboFei commented Jan 11, 2024

🔍 Description

Followup #5591
Support to get existing limit from more plan and regard the result max rows.

Issue References 🔗

This pull request fixes #

Describe Your Solution 🔧

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Types of changes 🔖

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests


Checklist 📝

Be nice. Be informative.

@turboFei turboFei changed the title Support to get limit from more spark plan and regard result max rows [KYUUBI #5377][FOLLOWUP] Support to get limit from more spark plan and regard result max rows Jan 11, 2024
@turboFei
Copy link
Member Author

@lsm1 @cxzl25 @pan3793

@turboFei turboFei requested review from cfmcgrady and pan3793 January 11, 2024 06:02
@turboFei turboFei force-pushed the incremental_save branch 3 times, most recently from ba264ff to 367622b Compare January 11, 2024 06:22
@turboFei turboFei requested a review from ulysses-you January 12, 2024 21:35
@turboFei turboFei changed the title [KYUUBI #5377][FOLLOWUP] Support to get limit from more spark plan and regard result max rows [KYUUBI #5377][FOLLOWUP] Get limit from logical plan and regard result max rows Jan 12, 2024
Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one minor suggestion

@turboFei turboFei self-assigned this Jan 15, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2024

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (4855ffe) 61.27% compared to head (223d510) 61.04%.
Report is 39 commits behind head on master.

Files Patch % Lines
...g/apache/spark/sql/kyuubi/SparkDatasetHelper.scala 12.50% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5963      +/-   ##
============================================
- Coverage     61.27%   61.04%   -0.23%     
  Complexity       23       23              
============================================
  Files           622      623       +1     
  Lines         36882    37150     +268     
  Branches       5014     5035      +21     
============================================
+ Hits          22599    22679      +80     
- Misses        11854    12014     +160     
- Partials       2429     2457      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@turboFei turboFei added this to the v1.8.1 milestone Jan 16, 2024
@turboFei
Copy link
Member Author

logical plan can not cover the case likes:

SELECT * FROM (SELECT * FROM tv ORDER BY id LIMIT $topKThreshold) AS TEMP

So, I revert to use sparkPlan.

@turboFei turboFei requested a review from ulysses-you January 16, 2024 00:27
@turboFei turboFei changed the title [KYUUBI #5377][FOLLOWUP] Get limit from logical plan and regard result max rows [KYUUBI #5377][FOLLOWUP] Get limit from spark plan and regard result max rows Jan 16, 2024
@turboFei turboFei changed the title [KYUUBI #5377][FOLLOWUP] Get limit from spark plan and regard result max rows [KYUUBI #5377][FOLLOWUP] Get limit from more spark plan and regard result max rows Jan 16, 2024
@ulysses-you
Copy link
Contributor

@turboFei can we use optimizedPlan ? SubqueryAlias should only at analyzed side.

@turboFei
Copy link
Member Author

addressed. @ulysses-you

@turboFei turboFei closed this in 576379c Feb 4, 2024
@turboFei turboFei deleted the incremental_save branch February 4, 2024 01:47
@turboFei
Copy link
Member Author

turboFei commented Feb 4, 2024

thanks, merged to master

zhaohehuhu pushed a commit to zhaohehuhu/incubator-kyuubi that referenced this pull request Feb 5, 2024
…ard result max rows

# 🔍 Description
Followup apache#5591
Support to get existing limit from more plan and regard the result max rows.
## Issue References 🔗

This pull request fixes #

## Describe Your Solution 🔧

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

## Types of changes 🔖

- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Test Plan 🧪

#### Behavior Without This Pull Request ⚰️

#### Behavior With This Pull Request 🎉

#### Related Unit Tests

---

# Checklist 📝

- [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

**Be nice. Be informative.**

Closes apache#5963 from turboFei/incremental_save.

Closes apache#5377

223d510 [Fei Wang] use optimized plan
ecefc2a [Fei Wang] use spark plan
57091e5 [Fei Wang] minor
2096144 [Fei Wang] for logical plan
0f734ee [Fei Wang] ut
fdc1155 [Fei Wang] save
f8e405a [Fei Wang] math.min

Authored-by: Fei Wang <[email protected]>
Signed-off-by: Fei Wang <[email protected]>
zhaohehuhu pushed a commit to zhaohehuhu/incubator-kyuubi that referenced this pull request Mar 21, 2024
…ard result max rows

# 🔍 Description
Followup apache#5591
Support to get existing limit from more plan and regard the result max rows.
## Issue References 🔗

This pull request fixes #

## Describe Your Solution 🔧

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

## Types of changes 🔖

- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Test Plan 🧪

#### Behavior Without This Pull Request ⚰️

#### Behavior With This Pull Request 🎉

#### Related Unit Tests

---

# Checklist 📝

- [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

**Be nice. Be informative.**

Closes apache#5963 from turboFei/incremental_save.

Closes apache#5377

223d510 [Fei Wang] use optimized plan
ecefc2a [Fei Wang] use spark plan
57091e5 [Fei Wang] minor
2096144 [Fei Wang] for logical plan
0f734ee [Fei Wang] ut
fdc1155 [Fei Wang] save
f8e405a [Fei Wang] math.min

Authored-by: Fei Wang <[email protected]>
Signed-off-by: Fei Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants